Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use memchr to optimize get_text_slice #344

Merged
merged 1 commit into from
May 5, 2024
Merged

Use memchr to optimize get_text_slice #344

merged 1 commit into from
May 5, 2024

Conversation

clubby789
Copy link
Contributor

Running the benchmarks on my machine, I get these results:

parse_ctx_runtime/browser                                                                            
                        time:   [106.61 µs 106.73 µs 106.85 µs]
                        change: [-17.886% -17.720% -17.552%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  8 (8.00%) high mild
  1 (1.00%) high severe
parse_ctx_runtime/preferences                                                                            
                        time:   [277.45 µs 277.83 µs 278.29 µs]
                        change: [-8.5355% -8.3601% -8.1949%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

With some stress tests against Fluent files used in rustc, I saw improvements of 20-30%.

@clubby789 clubby789 mentioned this pull request Feb 20, 2024
Closed
@clubby789
Copy link
Contributor Author

Some notable improvements when used in rustc:

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.3% [-0.3%, -0.2%] 3
Improvements ✅
(secondary)		 -1.5% [-4.1%, -0.3%] 41
All ❌✅ (primary) -0.3% [-0.3%, -0.2%] 3

@waywardmonkeys
Copy link
Collaborator

@flodolo You might want to approve the CI runs for this.

alerque
alerque approved these changes Mar 21, 2024
View reviewed changes
Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable. I think we're mostly waiting on getting a safe-harbor release out, then we can start moving forward with improvements. Thanks for the contribution.

@alerque alerque merged commit 2aa38ae into projectfluent:main May 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zbraniecki zbraniecki zbraniecki approved these changes

@alerque alerque alerque approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clubby789 @waywardmonkeys @alerque @zbraniecki